feat(nextjs): Extract tracing logic from server component wrapper templates #18408
Conversation
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| const isolationScope = getIsolationScope(); | ||
| const span = getActiveSpan(); | ||
| const { componentRoute, componentType } = context; | ||
| isolationScope.setTransactionName(`${componentType} Server Component (${componentRoute})`); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
|
||
| return startSpanManual( | ||
| { | ||
| op: 'function.nextjs', | ||
| name: `${componentType} Server Component (${componentRoute})`, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs.server_component', | ||
| 'sentry.nextjs.ssr.function.type': componentType, | ||
| 'sentry.nextjs.ssr.function.route': componentRoute, | ||
| }, | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Bug: Missing isolation scope context breaks request metadata attachment
The refactored code retrieves an isolation scope via commonObjectToIsolationScope at line 29 and sets normalizedRequest metadata on it, but the handleCallbackErrors callback is not wrapped with withIsolationScope. When captureException is called in the error handler, getIsolationScope() at line 42 returns the current global isolation scope - not the one with the request metadata. This causes captured exceptions to be missing request context (like headers) that was set up earlier. The original code used withIsolationScope(isolationScope, ...) to ensure all nested code ran with the correct isolation scope.
There was a problem hiding this comment.
the scope already gets forked earlier
node-overhead report 🧳Note: This is a synthetic benchmark with a minimal express app and does not necessarily reflect the real-world performance impact in an application.
|
| } else if (isRedirectNavigationError(error)) { | ||
| shouldCapture = false; | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| } | ||
| } | ||
|
|
||
| return startSpanManual( | ||
| { | ||
| op: 'function.nextjs', | ||
| name: `${componentType} Server Component (${componentRoute})`, | ||
| attributes: { | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_SOURCE]: 'component', | ||
| [SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN]: 'auto.function.nextjs.server_component', | ||
| 'sentry.nextjs.ssr.function.type': componentType, | ||
| 'sentry.nextjs.ssr.function.route': componentRoute, | ||
| if (shouldCapture) { | ||
| captureException(error, { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| page, | ||
| }) => { | ||
| const serverTransactionEventPromise = waitForTransaction('nextjs-app-dir', async transactionEvent => { | ||
| console.log(transactionEvent?.transaction); |
There was a problem hiding this comment.
Bug: Debug console.log left in test code
A console.log(transactionEvent?.transaction) statement appears to have been left in the test code, likely from debugging. While this is in test code, it will produce unnecessary output during test runs.
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
this is just relevant for the error case
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| }); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
size-limit report 📦
|
logaretm
left a comment
logaretm
left a comment
There was a problem hiding this comment.
Nice one, looks like the only blocker is Next.js 13 not liking this
packages/nextjs/src/server/index.ts
Outdated
| } | ||
| }); | ||
|
|
||
| client?.on('spanStart', span => { |
There was a problem hiding this comment.
l: Since args match we could do:
client?.on('spanStart', handleOnSpanStart);Unless you want to add more stuff to it later, just a nitpick.
| const isolationScope = getIsolationScope(); | ||
|
|
||
| const headersDict = context.headers ? winterCGHeadersToDict(context.headers) : undefined; |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| handleCallbackErrors, | ||
| propagationContextFromHeaders, | ||
| Scope, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_ORIGIN, | ||
| SEMANTIC_ATTRIBUTE_SENTRY_SOURCE, | ||
| setCapturedScopesOnSpan, | ||
| SPAN_STATUS_ERROR, | ||
| SPAN_STATUS_OK, | ||
| startSpanManual, | ||
| winterCGHeadersToDict, | ||
| withIsolationScope, | ||
| withScope, | ||
| } from '@sentry/core'; | ||
| import { isNotFoundNavigationError, isRedirectNavigationError } from '../common/nextNavigationErrorUtils'; | ||
| import type { ServerComponentContext } from '../common/types'; | ||
| import { flushSafelyWithTimeout, waitUntil } from '../common/utils/responseEnd'; | ||
| import { TRANSACTION_ATTR_SENTRY_TRACE_BACKFILL } from './span-attributes-with-logic-attached'; | ||
| import { commonObjectToIsolationScope, commonObjectToPropagationContext } from './utils/tracingUtils'; | ||
|
|
||
| /** | ||
| * Wraps an `app` directory server component with Sentry error instrumentation. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| activeSpan.updateName(enhancedName); | ||
| activeSpan.setAttributes({ | ||
| 'sentry.nextjs.ssr.function.type': segment === PAGE_SEGMENT ? 'Page' : 'Layout', | ||
| 'sentry.nextjs.ssr.function.route': route, |
There was a problem hiding this comment.
Bug: Inconsistent type handling for route span attribute
The route variable from rootSpanAttributes[ATTR_HTTP_ROUTE] can be undefined or a non-string type. On line 174, this is properly handled with typeof route === 'string' ? route : '' for the span name. However, on line 178, route is passed directly to setAttributes as 'sentry.nextjs.ssr.function.route': route, which could set the attribute to undefined or an unexpected type instead of being omitted or defaulted consistently.
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: Navigation errors captured when no active span exists
The navigation error checks (isNotFoundNavigationError and isRedirectNavigationError) that set shouldCapture = false are inside the if (span) block. If getActiveSpan() returns null or undefined, the entire block is skipped and shouldCapture remains true. This causes navigation errors (not-found and redirect) to be incorrectly captured via captureException when there's no active span, whereas the old code never captured these navigation errors regardless of span existence.
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| }); | ||
| } |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| }, | ||
| }, | ||
| span => { | ||
| return handleCallbackErrors( | ||
| () => originalFunction.apply(thisArg, args), | ||
| error => { | ||
| // When you read this code you might think: "Wait a minute, shouldn't we set the status on the root span too?" | ||
| // The answer is: "No." - The status of the root span is determined by whatever status code Next.js decides to put on the response. | ||
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| captureException(error, { | ||
| mechanism: { | ||
| handled: false, | ||
| type: 'auto.function.nextjs.server_component', | ||
| }, | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| span.end(); | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| } | ||
| }, | ||
| () => { | ||
| waitUntil(flushSafelyWithTimeout()); | ||
| }, | ||
| ); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Bug: The refactoring removed propagation context processing from wrapServerComponentWithSentry for the edge runtime, breaking distributed tracing continuity from incoming requests.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The refactoring removed logic that processes sentry-trace and baggage headers for server components running in the edge runtime. The previous code explicitly called propagationContextFromHeaders() and scope.setPropagationContext(), but this logic was not moved to the new centralized handleOnSpanStart handler or any other location. Other wrappers like wrapGenerationFunctionWithSentry.ts retain this logic, indicating its removal was an oversight. This breaks distributed tracing, as trace context from incoming requests is no longer propagated, leading to disconnected traces for server components in the edge runtime.
💡 Suggested Fix
Reintroduce the logic to parse sentry-trace and baggage headers and set the propagation context on the scope within wrapServerComponentWithSentry when process.env.NEXT_RUNTIME === 'edge'. This can be done by calling propagationContextFromHeaders() and scope.setPropagationContext() as was done previously.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/nextjs/src/common/wrapServerComponentWithSentry.ts#L35-L75
Potential issue: The refactoring removed logic that processes `sentry-trace` and
`baggage` headers for server components running in the edge runtime. The previous code
explicitly called `propagationContextFromHeaders()` and `scope.setPropagationContext()`,
but this logic was not moved to the new centralized `handleOnSpanStart` handler or any
other location. Other wrappers like `wrapGenerationFunctionWithSentry.ts` retain this
logic, indicating its removal was an oversight. This breaks distributed tracing, as
trace context from incoming requests is no longer propagated, leading to disconnected
traces for server components in the edge runtime.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7618121
| if (isNotFoundNavigationError(error)) { | ||
| // We don't want to report "not-found"s | ||
| shouldCapture = false; | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'not_found' }); | ||
| } else if (isRedirectNavigationError(error)) { | ||
| // We don't want to report redirects | ||
| shouldCapture = false; | ||
| span.setStatus({ code: SPAN_STATUS_OK }); | ||
| } else { | ||
| span.setStatus({ code: SPAN_STATUS_ERROR, message: 'internal_error' }); | ||
| } |
There was a problem hiding this comment.
Bug: The refactored wrapServerComponentWithSentry no longer calls span.end(), which can lead to incomplete traces and lost span data for successful executions.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
In the refactored wrapServerComponentWithSentry, the active span is no longer explicitly ended. The previous implementation used startSpanManual and ensured span.end() was called in a finally block. The new code retrieves the active span via getActiveSpan() but omits the corresponding span.end() call upon completion. This violates the standard span lifecycle. As a result, spans for successful server component executions may not be correctly recorded or flushed, leading to incomplete traces and lost observability data.
💡 Suggested Fix
Ensure the active span is explicitly ended after the wrapped function executes. A potential fix is to get the active span before the function runs and call span.end() within the finally block of handleCallbackErrors, mirroring the pattern used in the previous implementation and other wrappers.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: packages/nextjs/src/common/wrapServerComponentWithSentry.ts#L46-L57
Potential issue: In the refactored `wrapServerComponentWithSentry`, the active span is
no longer explicitly ended. The previous implementation used `startSpanManual` and
ensured `span.end()` was called in a `finally` block. The new code retrieves the active
span via `getActiveSpan()` but omits the corresponding `span.end()` call upon
completion. This violates the standard span lifecycle. As a result, spans for successful
server component executions may not be correctly recorded or flushed, leading to
incomplete traces and lost observability data.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7618121
closes https://linear.app/getsentry/issue/JS-1207/remove-tracing-from-app-router-server-components-templates
closes #18307